fix(#269): bucket My Trades filter by live status, not the stale snapshot - #299
fix(#269): bucket My Trades filter by live status, not the stale snapshot#299codaMW wants to merge 3 commits into
Conversation
…tale snapshot The status filter bucketed each trade using the status loaded once from the DB snapshot (rawTradesProvider), while the row chip shows the live status from tradeStatusProvider. The snapshot only refreshes on pull-to-refresh / retry / after create-take, so status changes arriving via gift wrap / 38383 left the filter stale: a taken order stayed in the Pending bucket while its chip read Waiting Invoice. Bucket the filter with the same live status the chip uses, making it the single source of truth. filteredTradesWithOrderStateProvider now reads tradeStatusProvider per trade and buckets on that, falling back to the snapshot status only until the live status has loaded (so nothing briefly escapes the active filter). The list re-buckets live, so a trade moving Pending to Waiting Invoice leaves the Pending filter in real time, matching its chip. Tests: a trade whose snapshot is Pending but whose live status is Waiting Invoice is excluded from the Pending bucket and included in the Waiting Invoice bucket; the snapshot fallback is covered too. Verified on a physical device (Nokia C31). Note: the persisted DB row can still be stale for own orders (the 38383 ingest skips syncing Pending back, orders.rs:2873). This change fixes the user-visible mismatch regardless; the DB write-back is a separate data-correctness follow-up. Closes MostroP2P#269.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: WalkthroughTrade filtering now uses each trade’s live status when available. It falls back to the persisted snapshot status while live status loads. Tests cover stale snapshots, waiting-invoice classification, and fallback behavior. ChangesLive status filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant FilteredTradesProvider
participant tradeStatusProvider
participant _tradeInfoToItem
FilteredTradesProvider->>tradeStatusProvider: Read live trade status
tradeStatusProvider-->>FilteredTradesProvider: Return live status or loading state
FilteredTradesProvider->>_tradeInfoToItem: Pass live or persisted status bucket
_tradeInfoToItem-->>FilteredTradesProvider: Return filtered trade item
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/features/trades/filtered_trades_provider_test.dart`:
- Around line 100-154: Extend the live-status tests around
staleSnapshotContainer and primeLiveStatus with a controllable status stream.
Assert the trade initially appears in its first live-status bucket, emit a
second status, then verify it is removed from the old bucket and appears in the
new bucket, covering re-bucketing after a live transition.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 873404f7-36a0-4805-aa6c-c4b15707920b
📒 Files selected for processing (2)
lib/features/trades/providers/trades_providers.darttest/features/trades/filtered_trades_provider_test.dart
Add a test that drives a trade's live status with a controllable stream: assert it sits in the Pending bucket, emit waitingBuyerInvoice, then assert it leaves Pending and enters the Waiting Invoice bucket. Covers the live re-bucketing the filter fix relies on, beyond the initial-status and snapshot-fallback cases.
|
@Catrya this is ready for review. What it does: filteredTradesWithOrderStateProvider now buckets each trade by the live status from tradeStatusProvider (the same source the row chip uses) instead of the DB snapshot, so the chip and the filter share one source of truth. _tradeInfoToItem takes an optional statusOverride so the provider supplies the live bucket without duplicating the mapping. Because it watches each trade's live status, the list re-buckets in real time, and it falls back to the snapshot bucket only until the live status has loaded. Tests: stale-snapshot excluded from the old bucket, present in the live bucket, snapshot fallback before first poll, and a live-transition test (Pending -> Waiting Invoice) covering the re-bucketing. Verified on a physical device (Nokia C31). Scope note: I kept the orders.rs:2873 write-back out of this PR. The persisted row can still be stale for own orders, but the UI is correct either way now since it reads live status. I think that write-back is worth doing for data correctness, but it's a distinct Rust change with its own testing (taker-timeout republish), so it felt cleaner as a follow-up. Happy to pick it up next or file it separately, whichever you prefer. |
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Reviewed at a30e62b, merged locally against current main (45 commits behind, but it merges clean). On the merged result: flutter analyze reports no issues and 246 Dart tests pass.
Reviewed at a30e62b, merged locally against current main (45 commits behind, but it merges clean). On the merged result: flutter analyze reports no issues and 246 Dart tests pass.
The fix itself is right and minimal, and I'd approve it as is were it not for one cost it introduces.
What holds up
The diagnosis is correct, and so is the note about the data layer: for own orders the 38383 ingest doesn't sync Pending back (orders.rs:2873), so the persisted row can stay stale. Fixing the user-visible mismatch by reading the live status is the right call, and keeping the write-back out of scope is the right split.
The tests cover what matters — snapshot-Pending / live-WaitingInvoice excluded from one bucket and included in the other, plus the fallback while the live status hasn't loaded.
Blocking: the fan-out gets pinned for the life of the process
filteredTradesWithOrderStateProvider is a plain FutureProvider — not autoDispose. tradeStatusProvider is a StreamProvider.family.autoDispose that polls every 2 s, calling orders_api.getOrder(orderId) over the bridge and, when the order is no longer in the order book, falling back to orders_api.listTrades() — a full table read.
Having the non-autoDispose provider watch that family for every trade in the list pins those family members: they are never disposed again. Before this change only the visible rows polled, and they were released when scrolled off. After it, every non-terminal trade in the list polls every 2 s for the rest of the process, whatever screen the user is on.
There's a precedent for the guard in the same file: orderBookNotificationCountProvider walks the trades and skips terminal ones before watching (if (_terminalOrderStatuses.contains(trade.order.status)) continue;, around line 265). The new loop watches all of them, history included.
The cheap fix is the same guard: skip trades whose snapshot status is already terminal. That stays safe even while distrusting the snapshot, because status only ever moves toward terminal — a snapshot can lag behind, never run ahead. For those trades the chip won't change again, so nothing this PR fixes is lost.
Coordination with #303
#303 (feat(#272): push-first trade status, replacing the 2 s poll) rewrites trade_state_provider.dart — 115 lines — which is exactly the provider this now fans out over. They don't conflict textually (this one touches trades_providers.dart), but they interact: if #303 lands first the poll is gone and this objection disappears entirely.
So there are two acceptable paths: add the terminal-status guard here, or merge #303 first and land this behind it. Either one and I'm happy to approve.
Minor
ref.watch after an await inside a FutureProvider is a pattern Riverpod discourages. The practical risk is low here because the provider isn't autoDispose and so isn't torn down, and the project doesn't run riverpod_lint to flag it — but it's worth knowing it's there, particularly if that provider ever becomes autoDispose.
…ostroP2P#299 review) Catrya's review: filteredTradesWithOrderStateProvider is a plain (non-autoDispose) FutureProvider that ref.watch(tradeStatusProvider(...)) for every trade, pinning a 2s poller per trade for the process's whole life — including terminal trades that can never change status again. Guard the watch with the terminal-status set, mirroring the existing guard in orderBookNotificationCountProvider: a terminal trade uses its snapshot status (live == null -> snapshot bucket) instead of spawning a watcher. This is safe because order status only ever moves toward terminal, so a snapshot can lag but never run ahead — nothing MostroP2P#269 fixes is lost. Added a regression test: a terminal trade with a non-terminal live-status override still buckets by its snapshot (success), proving the override is never read. flutter analyze clean; 10/10 provider tests pass.
|
. Guarded the `ref.watch(tradeStatusProvider(...))` with `_terminalOrderStatuses`, mirroring `orderBookNotificationCountProvider`: a terminal trade uses its snapshot status (`live == null` -> snapshot bucket) instead of spawning a 2s poller. Safe because status only moves toward terminal a snapshot can lag but never run ahead so the #269 behaviour is preserved. Added a regression test: a terminal trade given a non-terminal live-status override still buckets by its snapshot (success), which proves the override is never read. 10/10 provider tests pass, `flutter analyze` clean. On the `ref.watch`-after-`await` you flagged: I left it for this fix since the provider isn't autoDispose (so that disposal footgun doesn't apply here) and reordering the await is out of scope for #269 happy to open a follow-up if you'd like it tracked. |
2 participants
Problem
Each row's status chip shows the live status (
tradeStatusProvider), but the filter dropdown bucketed trades using the status loaded once from the DB snapshot (rawTradesProvider->filteredTradesWithOrderStateProvider). The snapshot only refreshes on pull-to-refresh / retry / after create-take, so changing the filter did not reload it, and status changes arriving via gift wrap / 38383 did not invalidate it.Result: chip-vs-filter mismatches. A taken order stayed in the Pending bucket while its chip read Waiting Invoice.
Fix
Bucket the filter with the same live status the chip uses, making it the single source of truth.
filteredTradesWithOrderStateProvidernow readstradeStatusProviderper trade and buckets on that, falling back to the snapshot status only until the live status has loaded (so nothing briefly escapes the active filter).Because the provider watches each trade's live status, the list re-buckets in real time: a trade moving Pending to Waiting Invoice leaves the Pending filter immediately, matching its chip.
_tradeInfoToItemgained an optionalstatusOverrideso the provider can supply the live bucket without duplicating the mapping.Testing
flutter analyzeclean; verified on a physical device (Nokia C31): whatever a trade's chip shows, selecting that status in the filter shows it and selecting a different status hides it.Note on the DB layer
For own orders the 38383 ingest skips syncing
Pendingback (orders.rs:2873), so the persisted row can remain stale. This change fixes the user-visible mismatch regardless (the live status bypasses the stale snapshot). The persisted-status write-back is a separate data-correctness item and would be a good follow-up; happy to take it on if you'd like it in-scope.Closes #269.
Summary by CodeRabbit
Bug Fixes
Tests